Skip to content

fix(go): resolve cross-module calls in multi-module layouts - #1521

Open
ericquan8 wants to merge 1 commit into
colbymchenry:mainfrom
ericquan8:fix/go-multi-module-resolution
Open

fix(go): resolve cross-module calls in multi-module layouts#1521
ericquan8 wants to merge 1 commit into
colbymchenry:mainfrom
ericquan8:fix/go-multi-module-resolution

Conversation

@ericquan8

Copy link
Copy Markdown

A Go monorepo with several independent modules under one root (and no root
go.mod) resolved cross-module calls at ~4% recall / ~7% precision: the
single-module reader only checked projectRoot/go.mod, so every in-repo
import was classified third-party and resolution fell through to global
name-matching, wiring most cross-module calls to the wrong target. A wrong
edge is worse than a missing one for impact analysis.

This is the follow-up #388's own code comment asked for.

Multi-module import resolution:

  • loadGoModules(): index EVERY go.mod under projectRoot (skip-list,
    depth/count caps, entries sorted by modulePath length DESCENDING so the
    longest prefix wins, deterministic traversal, shortest-relDir wins on a
    duplicated module path). packageDir() reproduces the old single-module
    algorithm byte-for-byte when relDir=''; it can legally return '', so
    callers test === null. loadGoModule()/GoModule untouched.
  • ResolutionContext.getGoModules?() added (optional); wired as a lazy,
    memoised provider alongside getGoModule(), matching the existing
    path-aliases / workspace-packages convention. Like those, it is resolver
    metadata only — it produces no nodes and no edges.
  • isExternalImport (Go branch), resolveGoCrossPackageReference and the
    name-matcher Go field-type guard prefer the index and fall back to the
    single-module path verbatim. The exact-parent match (fileDir === pkgDir)
    and the Go: external receiver calls resolve to unrelated local interface methods #1276 anti-fabrication guard are preserved.

Two extraction-layer defects with the same symptom (both reproduce in a
single-module repo too):

  • Go const/var/method isExported was never set, so the resolver's
    if (!node.isExported) continue dropped them. extractMethod never called
    the hook; the Go const/var branch omitted it AND would have fed the hook
    the declaration node, which has no name field — the identifier is on the
    const_spec/var_spec child. Computed per-spec inside the Go branch; the
    go.ts hook and the shared declaration-level isExported are untouched.
  • Grouped var (...) produced zero nodes. tree-sitter-go wraps a grouped
    var's specs in a var_spec_list node while a grouped const's specs are
    direct children, so the direct-child filter found nothing. Flattening the
    wrapper also restores calls made inside package-level grouped-var
    initializers, which previously had no source node at all.

Verification (reproducible) — 14 tests build real multi-module layouts in
temp dirs and index them for real, no mocks:

npx vitest run __tests__/resolution.test.ts -t "Go multi-module"
npx vitest run __tests__/extraction.test.ts -t "Go const/var extraction"

covering cross-module resolution, same-name symbol in a non-imported module
must not be picked, longest-prefix precedence, single-root-module
no-regress, no-go.mod no-op, sub-package exclusion, scan-depth cap, grouped
var/const extraction, isExported across all four const/var forms, and
TS/Python/Rust no-regress. Full suite green.

Scale check (private repo, NOT reproducible here; reported for magnitude).
11.5k Go files, 61 side-by-side modules. Ground truth is derived from the
source with no manual labelling — an import alias binds to one module path,
which maps to one local directory; a target defined exactly once in that
directory is unambiguous. Anything ambiguous is discarded, so the numbers
are conservative.

cross-module call recall        4.03%  ->  99.67%
cross-module target precision   6.83%  ->  100%
file coverage                  99.94%  ->  99.94%
call-site line precision       99.20%  ->  99.73%
node count                    302,900  -> 311,347  (+2.8%, grouped vars)

Known gap, out of scope: cross-module references to package-level const/var
still do not resolve. Value-position alias.Symbol references are never
EXTRACTED as references (flushValueRefs is same-file only by design), so
they never reach the resolver regardless of isExported. The isExported fix
above is still correct, but it removes a guard nothing currently reaches.
The real fix is an extraction-layer change, tracked separately.

Design notes: docs/design/go-multi-module-resolution.md

Refs #388.

A Go monorepo with several independent modules under one root (and no root
go.mod) resolved cross-module calls at ~4% recall / ~7% precision: the
single-module reader only checked projectRoot/go.mod, so every in-repo
import was classified third-party and resolution fell through to global
name-matching, wiring most cross-module calls to the wrong target. A wrong
edge is worse than a missing one for impact analysis.

This is the follow-up colbymchenry#388's own code comment asked for.

Multi-module import resolution:
- loadGoModules(): index EVERY go.mod under projectRoot (skip-list,
  depth/count caps, entries sorted by modulePath length DESCENDING so the
  longest prefix wins, deterministic traversal, shortest-relDir wins on a
  duplicated module path). packageDir() reproduces the old single-module
  algorithm byte-for-byte when relDir=''; it can legally return '', so
  callers test === null. loadGoModule()/GoModule untouched.
- ResolutionContext.getGoModules?() added (optional); wired as a lazy,
  memoised provider alongside getGoModule(), matching the existing
  path-aliases / workspace-packages convention. Like those, it is resolver
  metadata only — it produces no nodes and no edges.
- isExternalImport (Go branch), resolveGoCrossPackageReference and the
  name-matcher Go field-type guard prefer the index and fall back to the
  single-module path verbatim. The exact-parent match (fileDir === pkgDir)
  and the colbymchenry#1276 anti-fabrication guard are preserved.

Two extraction-layer defects with the same symptom (both reproduce in a
single-module repo too):
- Go const/var/method isExported was never set, so the resolver's
  `if (!node.isExported) continue` dropped them. extractMethod never called
  the hook; the Go const/var branch omitted it AND would have fed the hook
  the declaration node, which has no `name` field — the identifier is on the
  const_spec/var_spec child. Computed per-spec inside the Go branch; the
  go.ts hook and the shared declaration-level isExported are untouched.
- Grouped `var (...)` produced zero nodes. tree-sitter-go wraps a grouped
  var's specs in a `var_spec_list` node while a grouped const's specs are
  direct children, so the direct-child filter found nothing. Flattening the
  wrapper also restores calls made inside package-level grouped-var
  initializers, which previously had no source node at all.

Verification (reproducible) — 14 tests build real multi-module layouts in
temp dirs and index them for real, no mocks:

    npx vitest run __tests__/resolution.test.ts -t "Go multi-module"
    npx vitest run __tests__/extraction.test.ts -t "Go const/var extraction"

covering cross-module resolution, same-name symbol in a non-imported module
must not be picked, longest-prefix precedence, single-root-module
no-regress, no-go.mod no-op, sub-package exclusion, scan-depth cap, grouped
var/const extraction, isExported across all four const/var forms, and
TS/Python/Rust no-regress. Full suite green.

Scale check (private repo, NOT reproducible here; reported for magnitude).
11.5k Go files, 61 side-by-side modules. Ground truth is derived from the
source with no manual labelling — an import alias binds to one module path,
which maps to one local directory; a target defined exactly once in that
directory is unambiguous. Anything ambiguous is discarded, so the numbers
are conservative.

    cross-module call recall        4.03%  ->  99.67%
    cross-module target precision   6.83%  ->  100%
    file coverage                  99.94%  ->  99.94%
    call-site line precision       99.20%  ->  99.73%
    node count                    302,900  -> 311,347  (+2.8%, grouped vars)

Known gap, out of scope: cross-module references to package-level const/var
still do not resolve. Value-position `alias.Symbol` references are never
EXTRACTED as references (flushValueRefs is same-file only by design), so
they never reach the resolver regardless of isExported. The isExported fix
above is still correct, but it removes a guard nothing currently reaches.
The real fix is an extraction-layer change, tracked separately.

Design notes: docs/design/go-multi-module-resolution.md

Refs colbymchenry#388.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant